[PM-22236] Fix invited accounts stuck in intermediate claimed status #6810
Conversation
|
Claude finished @eliykat's task in 2m 42s —— View job Code Review SummaryI've completed my review of this PR. The implementation correctly addresses the defect where invited users with SummaryThis PR excludes invited users ( Verdict: APPROVED - No blocking issues found. All review feedback has been addressed. What Was ReviewedChanged Files:
Review Focus:
Positive Findings
Technical ValidationCorrectness of
Security assessment:
Notes
No inline comments needed - The implementation is clean and follows established patterns. Generated by Claude Code at commit |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6810 +/- ##
==========================================
+ Coverage 55.72% 59.64% +3.91%
==========================================
Files 1949 1949
Lines 86539 86541 +2
Branches 7725 7725
==========================================
+ Hits 48227 51615 +3388
+ Misses 36506 33039 -3467
- Partials 1806 1887 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
| SET NOCOUNT ON; | ||
|
|
||
| WITH OrgUsers AS ( | ||
| SELECT * |
There was a problem hiding this comment.
Just an observation: It looks like this is existing code, so it’s not your change. That said, in general I think we should move away from * and only select what we actually need.
There was a problem hiding this comment.
I agree! For now this returns a whole OrgUser object so we need to select the whole row, but we should move towards more targeted data objects.
I did find one unnecessary SELECT * that I removed though.
| od.VerifiedDate != null && | ||
| o.Enabled == true | ||
| o.Enabled == true && | ||
| ou.Status != OrganizationUserStatusType.Invited |
There was a problem hiding this comment.
Just thinking out loud here lol:
I’ve noticed we have a few cases where we exclude certain collection types or user types. I agree that we should do filtering in the database to leverage the query optimizer and reduce sending unnecessary data over the network. However, I wonder if we could configure the sproc's parameters to include only the types we want returned. That way, we can control the types from the app in the future, reduce the need for an extra deployment, and avoid potential backward compatibility issues.
That said, this does add upfront complexity to the sproc for a situation that might never happen.
There was a problem hiding this comment.
We do that in some sprocs, but here the status is closely tied to the purpose of the sproc: it's dealing with claimed accounts, and you can't claim an invited user's account. I would say this is a case where it should not be parameterized.
| WHERE OD.[VerifiedDate] IS NOT NULL | ||
| AND CU.EmailDomain = OD.[DomainName] | ||
| AND O.[Enabled] = 1 | ||
| AND OU.[Status] != 0 |
There was a problem hiding this comment.
It would be handy to have the status documented in a comment like you did in the other proc
https://contributing.bitwarden.com/contributing/code-style/sql/#comments-and-documentation
…aimed-account-in-invited-state-cannot-be-deleted-without-cs-assistance
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22236
📔 Objective
There is a known defect where user provisioning via the SSO
AccountControllerincorrectly creates anOrganizationUserin theInvitedstatus, but also links it to aUserId. This violates the expected contract forOrganizationUserobjects.This is generally not a problem because the user accepts the invite immediately and moves into a valid accepted state. However, it's not an atomic process, so if it goes off the happy path for any reason, the user will be left in this invalid invited state.
One side effect is that if the user's domain matches a domain claimed by the organization, they will be considered "claimed" in some contexts even though they are in the invited status. This is because we join through the
UserIdassuming that this will be null for invited OrganizationUsers; but the bug above invalidates this assumption. The result is that the user is stuck in a state that is simultaneously claimed and not claimed, and requires manual intervention by our CS team.The solution here is to expressly exclude invited users from these queries. The queries are:
Organization_ReadByClaimedUserEmailDomain- generally used to block user-driven actions such as deleting their account or purging their vaultOrganizationUser_ReadByOrganizationIdWithClaimedDomains_V2- used to display the list of members in the Admin Console with the correct actionsNote that this does not help users stuck in this invalid state and then revoked. Because revoked is its own status, this requires a workaround to detect the underlying status - probably checking for revoked + userId not null + email not null, similar to the OrganizationService method. However that is creating more workarounds to clean up later so I would rather leave this unhandled for now, noting that it does not pose any security risks. Instead we will work with Auth team to fix this edge case and stop these invalid states from arising (Jira ticket).
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes